From 7b50408786ecf129383c258b98de9a2b3969b62c Mon Sep 17 00:00:00 2001
From: Omer Tuchfeld <omer@tuchfeld.dev>
Date: Thu, 31 Oct 2024 13:49:25 +0100
Subject: [PATCH] install: Add support for pulling LBIs during install

Partially solves #846

This adds a new `--bound-images` option to `bootc install` which will
allow the user to choose how they want to handle the retrieval of LBIs
into the target's container storage.

The existing behavior, which will stay the default, is `--bound-images
stored` which will resolve the LBIs and verify they exist in the
source's container storage before copying them into the target's
container storage.

The new behavior is `--bound-images pull` which will skip the resolution
step and directly pull the LBIs into the target's container storage.

The older `--skip-bound-images` option (previously hidden) is now
removed and replaced with the new (but still hidden) `--bound-images
skip` option.

Signed-off-by: Omer Tuchfeld <omer@tuchfeld.dev>
Signed-off-by: Colin Walters <walters@verbum.org>
---
 lib/src/boundimage.rs |   6 ++-
 lib/src/cli.rs        |   5 ++
 lib/src/install.rs    | 116 +++++++++++++++++++++++++++++++-----------
 3 files changed, 95 insertions(+), 32 deletions(-)

diff --git a/lib/src/boundimage.rs b/lib/src/boundimage.rs
index f5b552c3..bc78d538 100644
--- a/lib/src/boundimage.rs
+++ b/lib/src/boundimage.rs
@@ -108,6 +108,7 @@ pub(crate) fn query_bound_images(root: &Dir) -> Result<Vec<BoundImage>> {
 
 #[cfg(feature = "install")]
 impl ResolvedBoundImage {
+    #[context("resolving bound image {}", src.image)]
     pub(crate) async fn from_image(src: &BoundImage) -> Result<Self> {
         let proxy = containers_image_proxy::ImageProxy::new().await?;
         let img = proxy
@@ -148,7 +149,10 @@ fn parse_container_file(file_contents: &tini::Ini) -> Result<BoundImage> {
 }
 
 #[context("Pulling bound images")]
-pub(crate) async fn pull_images(sysroot: &Storage, bound_images: Vec<BoundImage>) -> Result<()> {
+pub(crate) async fn pull_images(
+    sysroot: &Storage,
+    bound_images: Vec<crate::boundimage::BoundImage>,
+) -> Result<()> {
     tracing::debug!("Pulling bound images: {}", bound_images.len());
     // Yes, the usage of NonZeroUsize here is...maybe odd looking, but I find
     // it an elegant way to divide (empty vector, non empty vector) since
diff --git a/lib/src/cli.rs b/lib/src/cli.rs
index 1a9d47de..9f19a6a6 100644
--- a/lib/src/cli.rs
+++ b/lib/src/cli.rs
@@ -1039,6 +1039,11 @@ fn test_parse_install_args() {
     };
     assert!(o.target_opts.target_no_signature_verification);
     assert_eq!(o.filesystem_opts.root_path.as_str(), "/target");
+    // Ensure we default to old bound images behavior
+    assert_eq!(
+        o.config_opts.bound_images,
+        crate::install::BoundImagesOpt::Stored
+    );
 }
 
 #[test]
diff --git a/lib/src/install.rs b/lib/src/install.rs
index 1893c0a7..b3911562 100644
--- a/lib/src/install.rs
+++ b/lib/src/install.rs
@@ -21,7 +21,6 @@ use std::sync::Arc;
 use std::time::Duration;
 
 use anyhow::{anyhow, Context, Result};
-use anyhow::{ensure, Ok};
 use bootc_utils::CommandRunExt;
 use camino::Utf8Path;
 use camino::Utf8PathBuf;
@@ -44,6 +43,7 @@ use rustix::fs::{FileTypeExt, MetadataExt as _};
 use serde::{Deserialize, Serialize};
 
 use self::baseline::InstallBlockDeviceOpts;
+use crate::boundimage::{BoundImage, ResolvedBoundImage};
 use crate::containerenv::ContainerExecutionInfo;
 use crate::lsm;
 use crate::mount::Filesystem;
@@ -131,6 +131,27 @@ pub(crate) struct InstallSourceOpts {
     pub(crate) source_imgref: Option<String>,
 }
 
+#[derive(ValueEnum, Debug, Copy, Clone, PartialEq, Eq, Serialize, Deserialize, Default)]
+#[serde(rename_all = "kebab-case")]
+pub(crate) enum BoundImagesOpt {
+    /// Bound images must exist in the source's root container storage (default)
+    #[default]
+    Stored,
+    #[clap(hide = true)]
+    /// Do not resolve any "logically bound" images at install time.
+    Skip,
+    // TODO: Once we implement https://github.com/containers/bootc/issues/863 update this comment
+    // to mention source's root container storage being used as lookaside cache
+    /// Bound images will be pulled and stored directly in the target's bootc container storage
+    Pull,
+}
+
+impl std::fmt::Display for BoundImagesOpt {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        self.to_possible_value().unwrap().get_name().fmt(f)
+    }
+}
+
 #[derive(clap::Args, Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
 pub(crate) struct InstallConfigOpts {
     /// Disable SELinux in the target (installed) system.
@@ -166,10 +187,11 @@ pub(crate) struct InstallConfigOpts {
     #[serde(default)]
     pub(crate) generic_image: bool,
 
-    /// Do not pull any "logically bound" images at install time.
-    #[clap(long, hide = true)]
+    /// How should logically bound images be retrieved.
+    #[clap(long)]
     #[serde(default)]
-    pub(crate) skip_bound_images: bool,
+    #[arg(default_value_t)]
+    pub(crate) bound_images: BoundImagesOpt,
 
     /// The stateroot name to use. Defaults to `default`.
     #[clap(long)]
@@ -630,7 +652,7 @@ async fn initialize_ostree_root(state: &State, root_setup: &RootSetup) -> Result
     sysroot.load(cancellable)?;
 
     let stateroot_exists = rootfs_dir.try_exists(format!("ostree/deploy/{stateroot}"))?;
-    ensure!(
+    anyhow::ensure!(
         !stateroot_exists,
         "Cannot redeploy over extant stateroot {stateroot}"
     );
@@ -1318,7 +1340,7 @@ async fn install_with_sysroot(
     rootfs: &RootSetup,
     sysroot: &Storage,
     boot_uuid: &str,
-    bound_images: &[crate::boundimage::ResolvedBoundImage],
+    bound_images: BoundImages,
     has_ostree: bool,
 ) -> Result<()> {
     // And actually set up the container in that root, returning a deployment and
@@ -1346,18 +1368,66 @@ async fn install_with_sysroot(
     tracing::debug!("Installed bootloader");
 
     tracing::debug!("Perfoming post-deployment operations");
-    // Note that we *always* initialize this container storage, even
-    // if there are no bound images today.
+
+    // Note that we *always* initialize this container storage, even if there are no bound images
+    // today.
     let imgstore = sysroot.get_ensure_imgstore()?;
-    // Now copy each bound image from the host's container storage into the target.
-    for image in bound_images {
-        let image = image.image.as_str();
-        imgstore.pull_from_host_storage(image).await?;
+
+    match bound_images {
+        BoundImages::Skip => {}
+        BoundImages::Resolved(resolved_bound_images) => {
+            // Now copy each bound image from the host's container storage into the target.
+            for image in resolved_bound_images {
+                let image = image.image.as_str();
+                imgstore.pull_from_host_storage(image).await?;
+            }
+        }
+        BoundImages::Unresolved(bound_images) => {
+            crate::boundimage::pull_images(sysroot, bound_images)
+                .await
+                .context("pulling bound images")?;
+        }
     }
 
     Ok(())
 }
 
+enum BoundImages {
+    Skip,
+    Resolved(Vec<ResolvedBoundImage>),
+    Unresolved(Vec<BoundImage>),
+}
+
+impl BoundImages {
+    async fn from_state(state: &State) -> Result<Self> {
+        let bound_images = match state.config_opts.bound_images {
+            BoundImagesOpt::Skip => BoundImages::Skip,
+            others => {
+                let queried_images = crate::boundimage::query_bound_images(&state.container_root)?;
+                match others {
+                    BoundImagesOpt::Stored => {
+                        // Verify each bound image is present in the container storage
+                        let mut r = Vec::with_capacity(queried_images.len());
+                        for image in queried_images {
+                            let resolved = ResolvedBoundImage::from_image(&image).await?;
+                            tracing::debug!("Resolved {}: {}", resolved.image, resolved.digest);
+                            r.push(resolved)
+                        }
+                        BoundImages::Resolved(r)
+                    }
+                    BoundImagesOpt::Pull => {
+                        // No need to resolve the images, we will pull them into the target later
+                        BoundImages::Unresolved(queried_images)
+                    }
+                    BoundImagesOpt::Skip => anyhow::bail!("unreachable error"),
+                }
+            }
+        };
+
+        Ok(bound_images)
+    }
+}
+
 async fn install_to_filesystem_impl(state: &State, rootfs: &mut RootSetup) -> Result<()> {
     if matches!(state.selinux_state, SELinuxFinalState::ForceTargetDisabled) {
         rootfs.kargs.push("selinux=0".to_string());
@@ -1385,28 +1455,12 @@ async fn install_to_filesystem_impl(state: &State, rootfs: &mut RootSetup) -> Re
     tracing::debug!("boot uuid={boot_uuid}");
 
     // If we're doing an alongside install, then the /dev bootupd sees needs to be the host's.
-    ensure!(
+    anyhow::ensure!(
         crate::mount::is_same_as_host(Utf8Path::new("/dev"))?,
         "Missing /dev mount to host /dev"
     );
 
-    let bound_images = if state.config_opts.skip_bound_images {
-        Vec::new()
-    } else {
-        crate::boundimage::query_bound_images(&state.container_root)?
-    };
-    tracing::debug!("bound images={bound_images:?}");
-
-    // Verify each bound image is present in the container storage
-    let bound_images = {
-        let mut r = Vec::with_capacity(bound_images.len());
-        for image in bound_images {
-            let resolved = crate::boundimage::ResolvedBoundImage::from_image(&image).await?;
-            tracing::debug!("Resolved {}: {}", resolved.image, resolved.digest);
-            r.push(resolved)
-        }
-        r
-    };
+    let bound_images = BoundImages::from_state(state).await?;
 
     // Initialize the ostree sysroot (repo, stateroot, etc.)
     {
@@ -1416,7 +1470,7 @@ async fn install_to_filesystem_impl(state: &State, rootfs: &mut RootSetup) -> Re
             rootfs,
             &sysroot,
             &boot_uuid,
-            &bound_images,
+            bound_images,
             has_ostree,
         )
         .await?;
-- 
2.47.0